Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement PeerDAS Fulu fork activation #6795

Merged
merged 23 commits into from
Jan 30, 2025
Merged

Implement PeerDAS Fulu fork activation #6795

merged 23 commits into from
Jan 30, 2025

Conversation

jimmygchen
Copy link
Member

@jimmygchen jimmygchen commented Jan 13, 2025 •
edited
Loading

Issue Addressed

Addresses #6706

Proposed Changes

This PR activates PeerDAS at the Fulu fork epoch instead of EIP_7594_FORK_EPOCH. This means we no longer support testing PeerDAS with Deneb / Electrs, as it's now part of a hard fork.

Additional Info

Apologies for the large diff - it was initially a relatively small change, but ended up being bigger because:

  • Fulu tests were failing since we now activate PeerDAS at the Fulu fork, so I had to refactor test utils to support PeerDAS - the good news is that PeerDAS/Fulu now gets the same test coverage as the other forks \o/
  • The tests revealed a range sync bug where range requests were sent to custody peers selected from the global peer list, rather than peers from the same syncing chain.
  • This PR also changes Fulu to use *_payload_v4 engine methods, as the v5 are not yet implemented, we should revert this commit once we're ready to switch over (I've add TODO(fulu) comments to the changes): 8cdf82e
  • I've also introduced a ForkName::latest_stable() which returns Electra, so we don't run tests on Fulu by default.

@jimmygchen jimmygchen requested a review from jxs as a code owner January 13, 2025 05:17
@jimmygchen jimmygchen added the das Data Availability Sampling label Jan 13, 2025
@jimmygchen jimmygchen force-pushed the jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch branch from c1b0c91 to 2082c92 Compare January 13, 2025 05:39
@jimmygchen jimmygchen force-pushed the jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch branch from 2082c92 to b029342 Compare January 13, 2025 05:44
@jimmygchen jimmygchen added the ready-for-review The code is ready for review label Jan 13, 2025
@michaelsproul michaelsproul mentioned this pull request Jan 14, 2025
Closed
@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jan 14, 2025
@jimmygchen jimmygchen requested a review from dapplion January 14, 2025 07:30
@jimmygchen
Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-act…
0c9d64b 
…ivate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

# Conflicts:
#	beacon_node/lighthouse_network/src/rpc/protocol.rs
#	testing/ef_tests/check_all_files_accessed.py
#	testing/ef_tests/src/handler.rs
@jimmygchen
Address review comments and fix lint.
4e25302
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jan 15, 2025
@jimmygchen jimmygchen force-pushed the jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch branch 3 times, most recently from d188403 to 11e9e13 Compare January 15, 2025 04:43
@jimmygchen
Use engine v4 methods for Fulu (v5 methods do not exist yet). Update …
8cdf82e 
…kurtosis config for PeerDAS as electra genesis is not yet supported.
@jimmygchen jimmygchen force-pushed the jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch branch from 11e9e13 to 8cdf82e Compare January 15, 2025 04:57
@jimmygchen jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jan 15, 2025
@jimmygchen
Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-act…
4d407fe 
…ivate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

# Conflicts:
#	beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs
#	consensus/types/src/chain_spec.rs
#	testing/ef_tests/src/cases.rs
#	testing/ef_tests/src/cases/get_custody_groups.rs
#	testing/ef_tests/src/cases/kzg_compute_cells_and_kzg_proofs.rs
#	testing/ef_tests/src/cases/kzg_recover_cells_and_kzg_proofs.rs
#	testing/ef_tests/src/cases/kzg_verify_cell_kzg_proof_batch.rs
#	testing/ef_tests/src/handler.rs
#	testing/ef_tests/tests/tests.rs
jimmygchen added a commit to jimmygchen/lighthouse that referenced this pull request Jan 23, 2025
@jimmygchen
Implement PeerDAS Fulu fork activation (sigp#6795)
bb335bd 
Squashed commit of the following:

commit b3da74b
Merge: e813532 a1b7d61
Author: Jimmy Chen <[email protected]>
Date:   Thu Jan 23 16:11:34 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

    # Conflicts:
    #	beacon_node/beacon_chain/src/fetch_blobs.rs
    #	beacon_node/store/src/lib.rs
    #	beacon_node/store/src/memory_store.rs

commit e813532
Author: Jimmy Chen <[email protected]>
Date:   Tue Jan 21 17:44:19 2025 +1100

    Skip blob pruning tests for Fulu.

commit 0e8f671
Merge: 614f984 33c1648
Author: Jimmy Chen <[email protected]>
Date:   Tue Jan 21 16:03:53 2025 +1100

    Merge branch 'unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

commit 614f984
Author: Jimmy Chen <[email protected]>
Date:   Tue Jan 21 15:59:22 2025 +1100

    Fix range sync to select custody peers from its syncing chain instead of the global peer list.

commit eff9a5b
Author: Jimmy Chen <[email protected]>
Date:   Tue Jan 21 10:02:19 2025 +1100

    More test fixes for Fulu.

commit b63a6c4
Merge: b7da075 7a0388e
Author: Jimmy Chen <[email protected]>
Date:   Mon Jan 20 23:41:13 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

commit b7da075
Author: Jimmy Chen <[email protected]>
Date:   Mon Jan 20 16:03:36 2025 +1100

    More test fixes for Fulu.

commit 6d5b5ed
Merge: 8980832 06329ec
Author: Jimmy Chen <[email protected]>
Date:   Fri Jan 17 12:32:58 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

commit 8980832
Author: Jimmy Chen <[email protected]>
Date:   Fri Jan 17 11:41:46 2025 +1100

    Update Fulu spec tests. Revert back to testing Fulu as "feature", because all non-PeerDAS Fulu SSZ types are the same as Electra, and serde deserializes the vectors into Electra types.

commit 4d407fe
Merge: 8cdf82e b1a19a8
Author: Jimmy Chen <[email protected]>
Date:   Thu Jan 16 01:05:04 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

    # Conflicts:
    #	beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs
    #	consensus/types/src/chain_spec.rs
    #	testing/ef_tests/src/cases.rs
    #	testing/ef_tests/src/cases/get_custody_groups.rs
    #	testing/ef_tests/src/cases/kzg_compute_cells_and_kzg_proofs.rs
    #	testing/ef_tests/src/cases/kzg_recover_cells_and_kzg_proofs.rs
    #	testing/ef_tests/src/cases/kzg_verify_cell_kzg_proof_batch.rs
    #	testing/ef_tests/src/handler.rs
    #	testing/ef_tests/tests/tests.rs

commit 8cdf82e
Author: Jimmy Chen <[email protected]>
Date:   Wed Jan 15 14:31:29 2025 +1100

    Use engine v4 methods for Fulu (v5 methods do not exist yet). Update kurtosis config for PeerDAS as electra genesis is not yet supported.

commit 4e25302
Author: Jimmy Chen <[email protected]>
Date:   Wed Jan 15 13:07:43 2025 +1100

    Address review comments and fix lint.

commit 0c9d64b
Merge: 64e44e1 587c3e2
Author: Jimmy Chen <[email protected]>
Date:   Wed Jan 15 12:48:27 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

    # Conflicts:
    #	beacon_node/lighthouse_network/src/rpc/protocol.rs
    #	testing/ef_tests/check_all_files_accessed.py
    #	testing/ef_tests/src/handler.rs

commit 64e44e1
Author: Jimmy Chen <[email protected]>
Date:   Tue Jan 14 14:45:09 2025 +1100

    Fix failing tests now `fulu` fork is included.

commit b029342
Author: Jimmy Chen <[email protected]>
Date:   Mon Jan 13 16:30:34 2025 +1100

    Fix compilation and update Kurtosis test config for PeerDAS.

commit cd77b2c
Author: Jimmy Chen <[email protected]>
Date:   Mon Jan 13 16:16:18 2025 +1100

    Update spec tests.

commit 2e11554
Author: Jimmy Chen <[email protected]>
Date:   Mon Jan 13 14:45:55 2025 +1100

    Implement PeerDAS Fulu fork activation.
@jimmygchen jimmygchen force-pushed the jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch branch from 98863da to 492c1c6 Compare January 24, 2025 02:14
jimmygchen added a commit that referenced this pull request Jan 24, 2025
@jimmygchen
Implement PeerDAS Fulu fork activation (#6795)
048bacc 
Squashed commit of the following:

commit e21b31e
Author: Jimmy Chen <[email protected]>
Date:   Fri Jan 24 16:43:41 2025 +1100

    More beacon chain test fixes.

commit 492c1c6
Author: Jimmy Chen <[email protected]>
Date:   Fri Jan 24 13:00:44 2025 +1100

    Use pre-computed data columns for testing and fix tests.

commit b3da74b
Merge: e813532 a1b7d61
Author: Jimmy Chen <[email protected]>
Date:   Thu Jan 23 16:11:34 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

    # Conflicts:
    #	beacon_node/beacon_chain/src/fetch_blobs.rs
    #	beacon_node/store/src/lib.rs
    #	beacon_node/store/src/memory_store.rs

commit e813532
Author: Jimmy Chen <[email protected]>
Date:   Tue Jan 21 17:44:19 2025 +1100

    Skip blob pruning tests for Fulu.

commit 0e8f671
Merge: 614f984 33c1648
Author: Jimmy Chen <[email protected]>
Date:   Tue Jan 21 16:03:53 2025 +1100

    Merge branch 'unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

commit 614f984
Author: Jimmy Chen <[email protected]>
Date:   Tue Jan 21 15:59:22 2025 +1100

    Fix range sync to select custody peers from its syncing chain instead of the global peer list.

commit eff9a5b
Author: Jimmy Chen <[email protected]>
Date:   Tue Jan 21 10:02:19 2025 +1100

    More test fixes for Fulu.

commit b63a6c4
Merge: b7da075 7a0388e
Author: Jimmy Chen <[email protected]>
Date:   Mon Jan 20 23:41:13 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

commit b7da075
Author: Jimmy Chen <[email protected]>
Date:   Mon Jan 20 16:03:36 2025 +1100

    More test fixes for Fulu.

commit 6d5b5ed
Merge: 8980832 06329ec
Author: Jimmy Chen <[email protected]>
Date:   Fri Jan 17 12:32:58 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

commit 8980832
Author: Jimmy Chen <[email protected]>
Date:   Fri Jan 17 11:41:46 2025 +1100

    Update Fulu spec tests. Revert back to testing Fulu as "feature", because all non-PeerDAS Fulu SSZ types are the same as Electra, and serde deserializes the vectors into Electra types.

commit 4d407fe
Merge: 8cdf82e b1a19a8
Author: Jimmy Chen <[email protected]>
Date:   Thu Jan 16 01:05:04 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

    # Conflicts:
    #	beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs
    #	consensus/types/src/chain_spec.rs
    #	testing/ef_tests/src/cases.rs
    #	testing/ef_tests/src/cases/get_custody_groups.rs
    #	testing/ef_tests/src/cases/kzg_compute_cells_and_kzg_proofs.rs
    #	testing/ef_tests/src/cases/kzg_recover_cells_and_kzg_proofs.rs
    #	testing/ef_tests/src/cases/kzg_verify_cell_kzg_proof_batch.rs
    #	testing/ef_tests/src/handler.rs
    #	testing/ef_tests/tests/tests.rs

commit 8cdf82e
Author: Jimmy Chen <[email protected]>
Date:   Wed Jan 15 14:31:29 2025 +1100

    Use engine v4 methods for Fulu (v5 methods do not exist yet). Update kurtosis config for PeerDAS as electra genesis is not yet supported.

commit 4e25302
Author: Jimmy Chen <[email protected]>
Date:   Wed Jan 15 13:07:43 2025 +1100

    Address review comments and fix lint.

commit 0c9d64b
Merge: 64e44e1 587c3e2
Author: Jimmy Chen <[email protected]>
Date:   Wed Jan 15 12:48:27 2025 +1100

    Merge remote-tracking branch 'origin/unstable' into jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch

    # Conflicts:
    #	beacon_node/lighthouse_network/src/rpc/protocol.rs
    #	testing/ef_tests/check_all_files_accessed.py
    #	testing/ef_tests/src/handler.rs

commit 64e44e1
Author: Jimmy Chen <[email protected]>
Date:   Tue Jan 14 14:45:09 2025 +1100

    Fix failing tests now `fulu` fork is included.

commit b029342
Author: Jimmy Chen <[email protected]>
Date:   Mon Jan 13 16:30:34 2025 +1100

    Fix compilation and update Kurtosis test config for PeerDAS.

commit cd77b2c
Author: Jimmy Chen <[email protected]>
Date:   Mon Jan 13 16:16:18 2025 +1100

    Update spec tests.

commit 2e11554
Author: Jimmy Chen <[email protected]>
Date:   Mon Jan 13 14:45:55 2025 +1100

    Implement PeerDAS Fulu fork activation.
dapplion
dapplion requested changes Jan 25, 2025
View reviewed changes
Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fulu changes look good! I would remove the fix for selecting relevant peers in range sync and handle that in a separate PR

beacon_node/beacon_chain/src/beacon_chain.rs Outdated Show resolved Hide resolved
beacon_node/beacon_chain/src/beacon_chain.rs Show resolved Hide resolved
beacon_node/beacon_chain/tests/store_tests.rs Outdated Show resolved Hide resolved
beacon_node/network/src/service.rs Show resolved Hide resolved
beacon_node/network/src/sync/network_context.rs Outdated Show resolved Hide resolved
beacon_node/network/src/sync/network_context.rs Outdated Show resolved Hide resolved
@dapplion dapplion force-pushed the jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch branch from 38311f8 to d8cba4b Compare January 28, 2025 14:14
@dapplion
Copy link
Collaborator

I have reverted the change that draws custody peers from the chain's peer set. The test pause_and_resume_on_ee_offline broke as a result.

To fix it I followed a different path than you did initially @jimmygchen. I see why you needed to do the change to only draw peers from the sync chain but that's not the root cause of the failing test.

Let's break down pause_and_resume_on_ee_offline

  • Add peer A and create head chain A
  • Stop engine so nothing is sent for processing
  • Request to download blocks + columns on epoch 0 chain A
  • Complete the requests above, all sent to peer A
  • Chain A sends requests to peer A for epoch 1 chain A
  • Add peer B and create finalized chain B
  • Request to download blocks + columns on epoch 0 chain B - Now because we draw from the global pool there's a high chance that the custody requests will be split between peer A and peer B
  • Complete the requests above, but because we filter requests to peer B the batch download will not be complete
  • Enable engine, and expect batch epoch 0 chain A to be sent for processing (Ok, happens) + expect batch epoch 0 chain B to be sent for processing (failure, it's still on downloading mode)

To fix the tests I have made the find_blocks_by_range_request smarter with a better filter such that I can identify all column requests for a given chain even if split among different peers. We will need this improvement anyway when adding more tests. It's unrealistic for chains to have a single peer and in real networks custody by range requests are split among a lot of peers.

I also had to add an epoch filter as the extra request from chain A for epoch 1 was colliding with the complete attempt for chain B. As we add more tests we can make this filters more robust but I think it's good for now.

@dapplion dapplion self-requested a review January 28, 2025 19:18
jxs
jxs reviewed Jan 29, 2025
View reviewed changes
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jimmy, the network parts look good to me!

@pawanjay176 pawanjay176 added under-review A reviewer has only partially completed a review. and removed ready-for-review The code is ready for review labels Jan 30, 2025
pawanjay176
pawanjay176 approved these changes Jan 30, 2025
View reviewed changes
Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, mostly nits

beacon_node/lighthouse_network/src/rpc/protocol.rs Outdated Show resolved Hide resolved
beacon_node/lighthouse_network/src/rpc/protocol.rs Outdated Show resolved Hide resolved
beacon_node/network/src/service.rs Show resolved Hide resolved
jimmygchen
jimmygchen commented Jan 30, 2025
View reviewed changes
Copy link
Member Author

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dapplion your change looks good to me!

@jimmygchen jimmygchen force-pushed the jimmy/lh-2271-activate-peerdas-at-fulu-fork-and-remove-eip7594_fork_epoch branch from 5f70c41 to ce8090d Compare January 30, 2025 04:44
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed under-review A reviewer has only partially completed a review. labels Jan 30, 2025
AgeManning
AgeManning approved these changes Jan 30, 2025
View reviewed changes
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretend review, lgtm

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jan 30, 2025
mergify bot added a commit that referenced this pull request Jan 30, 2025
@mergify
Merge of #6795
de98f16
@mergify mergify bot mentioned this pull request Jan 30, 2025
Closed
10 tasks
@mergify mergify bot merged commit 70194df into sigp:unstable Jan 30, 2025
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jxs jxs jxs left review comments

@mergify mergify[bot] mergify[bot] approved these changes

@dapplion dapplion dapplion approved these changes

@pawanjay176 pawanjay176 pawanjay176 approved these changes

@AgeManning AgeManning AgeManning approved these changes

Assignees
No one assigned
Labels
das Data Availability Sampling ready-for-merge This PR is ready to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jimmygchen @dapplion @jxs @AgeManning @pawanjay176